Skip to content

Conversation

@trinistr
Copy link
Contributor

@trinistr trinistr commented Nov 6, 2025

From #1265: new anonymous: argument to Tempfile.create.

Turns out that there was no spec for the method at all! But there was an empty spec for Tempfile.callback, which doesn't exist.

Also, there is a spec for Tempfile#_close, which is a protected method and seems like an implementation detail. I think that that spec should be deleted.

@trinistr trinistr marked this pull request as draft November 6, 2025 21:34
@trinistr trinistr marked this pull request as ready for review November 6, 2025 21:49
@trinistr trinistr changed the title Add spec for Tempfile.create [Ruby 3.4] Add spec for Tempfile.create Nov 7, 2025
@tempfile = Tempfile.create(anonymous: true)
@tempfile.should_not.closed?
@tempfile.path.should == "#{Dir.tmpdir}/"
File.file?(@tempfile.path).should be_false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: If you are agree to use File.exist? then there are still File.file? calls left in the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those left over are intentional, as @tempfile.path may refer to Dir.tmpdir, which exists but not a file, and in the first test checks that it's not a FIFO or something. Though I'm now not sure that using exist? is even better, because it may cause more questions.

@trinistr trinistr changed the title [Ruby 3.4] Add spec for Tempfile.create [Ruby 3.4] Add spec for Tempfile.create and its new anonymous: argument Nov 7, 2025
@andrykonchin
Copy link
Member

Could you please squash code review changes (I suppose they are mostly related to the first commit)?

@trinistr
Copy link
Contributor Author

trinistr commented Nov 7, 2025

I'm not sure exactly how to better squash this? Two total commits (general spec and 3.4 spec)? One commit?

@trinistr trinistr force-pushed the 3.4-tempfile-create-anonymous branch from 3be91cf to 3523409 Compare November 7, 2025 15:38
@trinistr
Copy link
Contributor Author

trinistr commented Nov 7, 2025

@andrykonchin I've squashed changes into two commits. Please tell me if that's what you meant.

And thank you very much for the thorough reviews!

@andrykonchin
Copy link
Member

Thank you! It's exactly what I meant.

@andrykonchin andrykonchin merged commit 64a1a40 into ruby:master Nov 7, 2025
12 checks passed
@trinistr trinistr deleted the 3.4-tempfile-create-anonymous branch November 20, 2025 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants